Dyno: Type-convert select-statements #27095
Conversation
Signed-off-by: Ben Harshbarger <[email protected]>
Signed-off-by: Ben Harshbarger <[email protected]>
There was a problem hiding this comment.
This looks awesome, my only worry is that we should probably avoid constructing temps without types {} where possible (for now), e.g., for the storeInTempIfNeeded calls, as there are some situations where calling ->qualType() or ->typeInfo() can trigger the old resolver.
| Symbol* t = nullptr; | ||
|
|
||
| if (qt.isUnknown()) { | ||
| t = makeNewTemp(e->qualType().getQual(), e->typeInfo()); |
There was a problem hiding this comment.
The problem with this is that there are at least some paths through ->qualType() which seem to invoke the old resolver which is why I decided to only allow temps created with the chpl::types::QualifiedType. If we could figure out some way to translate without invoking the old resolver this could be done.
E.g., I know there are some primitives where the types are determined via the old resolver. I have not done due diligence but there could be more.
Maybe we hold off on adding this sort of thing if we can and if we do make sure via sanity checks/crashes that it can't actually invoke the old resolver.
There was a problem hiding this comment.
I was unaware that this was the case, so I'll look into using QualifiedType.
There was a problem hiding this comment.
Cool, thanks, maybe consider adding a comment or something so that people in the future can catch on? I thought I added one but it was probably in a bad spot or too long winded.
| @@ -4050,7 +4068,8 @@ void TConverter::ActualConverter::convertActual(const FormalActual& fa) { | |||
| INT_ASSERT(astActual); | |||
|
|
|||
| // Convert the actual and leave. | |||
| std::get<Expr*>(slot) = tc_->convertExpr(astActual, rv_); | |||
| auto actualExpr = tc_->convertExpr(astActual, rv_); | |||
| std::get<Expr*>(slot) = tc_->storeInTempIfNeeded(actualExpr, {}); | |||
There was a problem hiding this comment.
Do we always need to have the actuals be temps?
There was a problem hiding this comment.
The uAST here could be a Call*, and normalized production AST requires a temporary.
There was a problem hiding this comment.
Makes sense to me, thanks! Maybe we could have storeInTempIfNeeded be more sensitive so it can avoid that if possible, because currently it just always stores in a temp.
| @@ -5071,6 +5090,157 @@ bool TConverter::enter(const ExternBlock* node, RV& rv) { | |||
|
|
|||
| void TConverter::exit(const ExternBlock* node, RV& rv) {} | |||
|
|
|||
| static SymExpr* makeCaseCond(TConverter& tc, TConverter::RV& rv, | |||
There was a problem hiding this comment.
IMO we should normalize either TConverter* or TConverter& for static helper functions so that they're consistent, I don't really have a preference here but usually just do TConverter* cause then you can pass in this. Thoughts?
There was a problem hiding this comment.
I guess I prefer to use references when possible, but for consistency I'll change this.
There was a problem hiding this comment.
I'm fine with using references, I'd just want to go and change the other areas to use them too 😄 is all. So feel free to keep and I'll add a TODO.
| @@ -4050,7 +4068,8 @@ void TConverter::ActualConverter::convertActual(const FormalActual& fa) { | |||
| INT_ASSERT(astActual); | |||
|
|
|||
| // Convert the actual and leave. | |||
| std::get<Expr*>(slot) = tc_->convertExpr(astActual, rv_); | |||
| auto actualExpr = tc_->convertExpr(astActual, rv_); | |||
| std::get<Expr*>(slot) = tc_->storeInTempIfNeeded(actualExpr, {}); | |||
There was a problem hiding this comment.
See the TODO, I think it would be good to have the callsites supply the types unless we're sure we're not going to accidentally invoke the old resolver.
So just get the type from convertExpr for now?
| // it's a bit confusing for the case-expression's type in 'rv' to always be | ||
| // a bool. | ||
| Expr* caseExpr = tc.convertExpr(cs, rv); | ||
| caseExpr = tc.storeInTempIfNeeded(caseExpr, {}); |
There was a problem hiding this comment.
Can we get the type from convertExpr and pass that to storeInTempIfNeeded here?
frontend/lib/resolution/Resolver.h
Outdated
| @@ -37,7 +37,13 @@ namespace resolution { | |||
|
|
|||
| struct Resolver : BranchSensitiveVisitor<DefaultFrame> { | |||
| // types used below | |||
| using ActionAndId = std::tuple<AssociatedAction::Action, ID>; | |||
| //using ActionInfo = std::tuple<AssociatedAction::Action, ID, types::QualifiedType>; | |||
There was a problem hiding this comment.
nit: can remove now
Previously we were storing the boolean result of a when-statement's '==' operator in the ResolvedExpression for the case-expression. This was inconsistent with how we do things elsewhere, and caused complications in the typed converter. This commit adds a QualifiedType to AssociatedActions and stores the boolean result there. Storing this result is particularly important in the case where it is param-true or param-false. This commit also adds a 'getAction' method to ResolvedExpression to facilitate fetching the 'COMPARE' action. Signed-off-by: Ben Harshbarger <[email protected]>
Also update with reviewer feedback: - do not use 'qualType()' - consistent usage of 'TypedConverter*' Signed-off-by: Ben Harshbarger <[email protected]>
benharsh
force-pushed
the
convert-select
branch
from
99c4403 to
858a2fd
Compare
This PR adds initial support for type-converting select-statements using resolution info from dyno. Like production, this PR translates select-statements into if-statements.
One difference with production concerns a when-statement with multiple cases, where one of those cases is param-true. Historically we have executed a comparison for all case-exprs up to the param-true case. In this PR, we no longer execute those comparisons when any case-expr is param-true.
Testing: